Post Settings: Refactor revisions handling to avoid performance issues #3233

gziolo · 2017-10-30T10:35:02Z

Description

Fixes #3167 - Post edit script takes way much longer to generate for Gutenberg editor. With the help of @pento we were able to identify the root cause:

Performance improves significantly when I comment out this block:

https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L706-L708

This code preloads API response for revisions endpoint, which fetches all existing post revisions and exposes them in the <script /> tag. When you have let's say 50 revisions, all of them are loaded from the database and exposed in the HTML code leading to the big size of the HTML and processing time to create all objects.

I refactored the handling of revisions to extend REST API endpoint for post types to include information about the number of revisions and the last revision id.

It solves a few issues I noticed:

performance of the post edit page
number of revisions updates whenever new revision gets created together with the link
the link to the revision was pointing to the wrong revision

How Has This Been Tested?

Manually:

Create new post or page
Make sure they are no errors
Save draft
Make sure that link to the revision shows up and you can navigate to this page
Load existing post or page
Make sure that link to the revisions shows up and you can navigate to this page
Save another revision
Make sure that link to the revisions updates properly

Screenshots (jpeg or gifs if applicable):

Types of changes

Bug fix (non-breaking change which fixes an issue). Code was refactored.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

gziolo · 2017-10-30T10:36:33Z

lib/client-assets.php

@@ -462,12 +462,12 @@ function gutenberg_extend_wp_api_backbone_client() {
 				return model.prototype.route && route === model.prototype.route.index;
 			} );
 		};
-		wp.api.getPostTypeRevisionsCollection = function( postType ) {
+		/*wp.api.getPostTypeRevisionsCollection = function( postType ) {


We no longer need this handler, should we remove it?

If it's dead code, I'm in favor of removing it.

gziolo · 2017-10-30T10:37:46Z

lib/client-assets.php

@@ -703,10 +703,6 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
 		gutenberg_get_rest_link( $post_to_edit, 'about', 'edit' ),
 	);

-	if ( ! $is_new_post ) {


Preloading all revisions and exposing them in the Script tag was the main root cause of performance issues reported in #3167. Kudos to @pento for finding out 👍

See my description:

This code preloads API response for revisions endpoint, which fetches all existing post revisions and exposes them in the <script /> tag. When you have let's say 50 revisions, all of them are loaded from the database and exposed in the HTML code leading to the big size of the HTML and increases processing time to process all objects.

And more technical comment from @pento:

Loading all the revisions was running pretty much all of the code that would be run if the posts were being loaded to display on the front end. ‘the_content’ was the biggest problem, but everything added up.

Sorry, definitely overlooked this 👍

youknowriad · 2017-10-30T10:38:39Z

lib/register.php

+		)
+	);
+}
+add_action( 'rest_api_init', 'gutenberg_register_post_revisions' );


Just wondering if it's a good idea to add extra fields to this endpoint. This won't be specific to Gutenberg.

I'm learning WP hooks and APIs. I'm open to suggestions how to implement it better. This change makes loading Gutenberg with tons of revisions comparable to the Classic editor.

I still think a separate endpoint is better, do you know why the previously used endpoint was slow? Maybe we should create a custom endpoint? Also, I'm not 100% confident when it comes to the REST API, others might have a better input.

@youknowriad: Loading all the revisions was running pretty much all of the code that would be run if the posts were being loaded to display on the front end. the_content was the biggest problem, but everything added up.

I'm okay with the field being here for now. I was unfortunately waylaid for the past week or so, so I haven't managed to focus on the REST API problem, but I'm hoping to get back to it later this week. I imagine there'll be a lot of re-shuffling, so I wouldn't get too hung up on where this is for now.

Yes, I'm aware of that, I'm just saying we may want to load the "last_revision_id" and "revision_count" in a separate endpoint instead of adding these as extra fields to the default POST endpoint. Because this change will change the result for everything and not only Gutenberg.

gziolo · 2017-10-30T10:39:09Z

lib/register.php

+	);
+}
+
+function gutenberg_register_post_revisions() {


Docs are missing. I will add later.

aduth · 2017-10-30T18:30:05Z

While it doesn't appear that it will prevent content filtering from being run, an alternative solution to limiting the size of the version history payload in the upcoming 4.9 release is to specify only the necessary fields on the API request:

e.g. /wp/v2/posts/4462/revisions?_fields=id

See: https://core.trac.wordpress.org/ticket/38131

gziolo · 2017-10-31T07:42:08Z

It seems like this solution makes sense as a temporary workaround. I will clean up this branch and add proper documentation to the rest_api_init hook.

While it doesn't appear that it will prevent content filtering from being run, an alternative solution to limiting the size of the version history payload in the upcoming 4.9 release is to specify only the necessary fields on the API request:

@aduth: Yes, that would be one of the possible ways of improving performance here. I think it would still need more work to make sure that REST API call is triggered every time new post's content is saved. At the moment information about revisions doesn't get updated whenever a post is saved. This PR addresses it, too.

Yes, I'm aware of that, I'm just saying we may want to load the "last_revision_id" and "revision_count" in a separate endpoint instead of adding these as extra fields to the default POST endpoint. Because this change will change the result for everything and not only Gutenberg.

@youknowriad: it is another solution that would work. Again, we would need to find a way to refresh data each time post is saved.

@pento post data structure contains a list of categories or tags, why this is not the case for revisions? I'm sure there are reasons like we don't want to fetch everything and ruin performance. According to the documentation, we can pass context argument where one of the values is edit. It seems like a perfect match to our use case. So I would be happy to explore adding revisions to the REST API and enable it by default only in edit context. What do you think?

codecov · 2017-10-31T10:16:30Z

Codecov Report

Merging #3233 into master will increase coverage by 0.04%.
The diff coverage is 40%.

@@            Coverage Diff            @@
##           master   #3233      +/-   ##
=========================================
+ Coverage   31.16%   31.2%   +0.04%     
=========================================
  Files         229     229              
  Lines        6431    6428       -3     
  Branches     1145    1145              
=========================================
+ Hits         2004    2006       +2     
+ Misses       3716    3711       -5     
  Partials      711     711

Impacted Files	Coverage Δ
editor/sidebar/last-revision/index.js	`0% <0%> (ø)`	⬆️
editor/selectors.js	`95.26% <100%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80065ba...69fbe32. Read the comment docs.

youknowriad

Let's get this in (for the release) and make sure we revisit the endpoint later

gziolo · 2017-10-31T10:28:22Z

Thanks for all your feedback and reviews 🙇

I'll open a follow-up issue. Let's discuss further steps there.

gziolo added [Type] Performance Related to performance efforts [Feature] Document Settings Document settings experience labels Oct 30, 2017

gziolo self-assigned this Oct 30, 2017

gziolo requested review from youknowriad, pento and aduth October 30, 2017 10:35

gziolo commented Oct 30, 2017

View reviewed changes

youknowriad reviewed Oct 30, 2017

View reviewed changes

gziolo commented Oct 30, 2017

View reviewed changes

lib/register.php Outdated

);

}

function gutenberg_register_post_revisions() {

Copy link

Member Author

gziolo Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs are missing. I will add later.

gziolo requested a review from ellatrix October 30, 2017 10:51

gziolo changed the title ~~Post Settings: Refactor revisions handling to avoid performacne issues~~ Post Settings: Refactor revisions handling to avoid performance issues Oct 30, 2017

Post Settings: Refactor revisions handling to avoid performacne issues

69fbe32

gziolo force-pushed the fix/post-revisions branch from 2086951 to 69fbe32 Compare October 31, 2017 10:16

youknowriad approved these changes Oct 31, 2017

View reviewed changes

gziolo merged commit fcca1f2 into master Oct 31, 2017

gziolo deleted the fix/post-revisions branch October 31, 2017 10:27

This was referenced Oct 31, 2017

Extremely slow to show editor on longer pages #2822

Closed

Framework: Improve how post revisions are handled #3258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post Settings: Refactor revisions handling to avoid performance issues #3233

Post Settings: Refactor revisions handling to avoid performance issues #3233

gziolo commented Oct 30, 2017 •

edited

Loading

gziolo Oct 30, 2017

aduth Oct 30, 2017

gziolo Oct 30, 2017

aduth Oct 30, 2017

gziolo Oct 30, 2017

aduth Oct 30, 2017

youknowriad Oct 30, 2017

gziolo Oct 30, 2017

youknowriad Oct 30, 2017

pento Oct 30, 2017

pento Oct 30, 2017

youknowriad Oct 30, 2017

gziolo Oct 30, 2017

aduth commented Oct 30, 2017

gziolo commented Oct 31, 2017 •

edited

Loading

codecov bot commented Oct 31, 2017 •

edited

Loading

youknowriad left a comment

gziolo commented Oct 31, 2017

Post Settings: Refactor revisions handling to avoid performance issues #3233

Post Settings: Refactor revisions handling to avoid performance issues #3233

Conversation

gziolo commented Oct 30, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Oct 30, 2017

gziolo commented Oct 31, 2017 • edited Loading

codecov bot commented Oct 31, 2017 • edited Loading

Codecov Report

youknowriad left a comment

Choose a reason for hiding this comment

gziolo commented Oct 31, 2017

gziolo commented Oct 30, 2017 •

edited

Loading

gziolo commented Oct 31, 2017 •

edited

Loading

codecov bot commented Oct 31, 2017 •

edited

Loading